-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for the "module" package.json field #5485
Conversation
A single failing test, mind fixing it? Also failing flow |
Yes, I will work on this in a bit! |
So I got it to work for local modules, now trying to get it to work for imported modules. I've got it to work for modules that are imported like this already: import { connect } from 'react-redux'; But if you do something like this it won't work: import isPlainObject from 'lodash-es/isPlainObject'; This doesn't work yet, because I haven't come up with a way yet to track the modules that are resolved as files. Will revise this PR tomorrow, please let me know if you have any ideas! |
const resolveTarget = path.resolve(name, pkgentry); | ||
|
||
if (isRequiredByModuleField) { | ||
REQUIRED_BY_MODULE_FIELD_CACHE[name] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly I reverted to keeping a local CACHE singleton here, if you have any ideas, please let me know as well!
|
||
try { | ||
if (willTransform) { | ||
const transformedSource = this.transformSource( | ||
filename, | ||
content, | ||
instrument, | ||
options.isRequiredByModuleField, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, shouldTransform
is called twice, I just passed it through for now. Maybe someone from the jest team can check if it is actually necessary to call it twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Twice within the same file or same code path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same code path.
@SimenB any chance you have time checking this out with me? |
Could you rebase and fix the flow errors? Anything in particular you want answered? |
So mostly the problem is what I commented here: #5485 (comment). The story about deep imports. |
This is used in modern es module codebases to designate the entry point for code that makes use of import/export instead of commonjs.
Hi, any updates on this? |
I would love to see this merged. If there's a way I can help let me know! |
@dmk255 @CosminIonascu this PR works until the point where packages require other packages through the .module field. Like react-redux requiring lodash-es, I’ll be on holidays for a week now, pleass feel free to tinker with this PR! |
@PepijnSenders any chance you could wrap this up? |
@rickhanlonii I'm pretty busy in general, but I got stuck on the thing I mentioned above. Not sure how other system manage this, but it's pretty hard to differentiate which modules you can load with the |
are there any updates in this pr? I think it is super helpful if we can merge this one :D |
Does the Or, we can just land this without the deep support, that's better than nothing, isn't it? |
Bump |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Closes #2702
This is a duplicate of #2704.
Original description: